Convert units for bounds when compiling full state descriptions #3132
Conversation
So that other pieces of openhab can know what unit it's going to be, without it having a value yet. Importantly, any necessary conversion that need to be applied to the other portion of the state description - min, max, and step. See also openhab/openhab-core#3132 Signed-off-by: Cody Cutrer <cody@cutrer.us>
So that other pieces of openhab can know what unit it's going to be, without it having a value yet. Importantly, any necessary conversion that need to be applied to the other portion of the state description - min, max, and step. See also openhab/openhab-core#3132 Signed-off-by: Cody Cutrer <cody@cutrer.us>
ccutrer
force-pushed
the
convert-bounds-in-state-descriptions
branch
2 times, most recently
from
8ac94ab to
815b0d6
Compare
So that other pieces of openhab can know what unit it's going to be, without it having a value yet. Importantly, any necessary conversion that need to be applied to the other portion of the state description - min, max, and step. See also openhab/openhab-core#3132 Signed-off-by: Cody Cutrer <cody@cutrer.us>
So that other pieces of openhab can know what unit it's going to be, without it having a value yet. Importantly, any necessary conversion that need to be applied to the other portion of the state description - min, max, and step. See also openhab/openhab-core#3132 Signed-off-by: Cody Cutrer <cody@cutrer.us>
So that other pieces of openhab can know what unit it's going to be, without it having a value yet. Importantly, any necessary conversion that need to be applied to the other portion of the state description - min, max, and step. See also openhab/openhab-core#3132 Signed-off-by: Cody Cutrer <cody@cutrer.us> Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
|
I feel that - especially considering #2283 - we should discuss what get's merged and what SHOULD take precedence (not necessarily what we have now). |
|
+1. I hadn't even considered how multiple-linking would affect this. Definitely needs thought through |
Signed-off-by: Cody Cutrer <cody@cutrer.us>
ccutrer
force-pushed
the
convert-bounds-in-state-descriptions
branch
from
4083b5f to
86d0b43
Compare
|
I force-pushed and rebased since part of this PR was extracted to its own PR and merged. remaining files have not changed since the last version of this PR, and discussion is still pending. Just wanted to clean it up so when we get back to it in the future it won't be as confusing. |
wborn
changed the title
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/uom-default-units-and-consequences/142360/75 |
So that other pieces of openhab can know what unit it's going to be, without it having a value yet. Importantly, any necessary conversion that need to be applied to the other portion of the state description - min, max, and step. See also openhab/openhab-core#3132 Signed-off-by: Cody Cutrer <cody@cutrer.us>
So that other pieces of openhab can know what unit it's going to be, without it having a value yet. Importantly, any necessary conversion that need to be applied to the other portion of the state description - min, max, and step. See also openhab/openhab-core#3132 Signed-off-by: Cody Cutrer <cody@cutrer.us>
So that other pieces of openhab can know what unit it's going to be, without it having a value yet. Importantly, any necessary conversion that need to be applied to the other portion of the state description - min, max, and step. See also openhab/openhab-core#3132 Signed-off-by: Cody Cutrer <cody@cutrer.us>
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/binding-improvements-best-practice/146664/2 |
|
@ccutrer Now that we have proper UoM support, could you rebase and check if this is still working as you expect? If so, I can review it. |
J-N-K
added
enhancement
There was a problem hiding this comment.
Thanks, mostly looks good. Can you also add. Test that includes Number:Dimensionless? We have seen a lot of unexpected issues with the in the past, it would be good to have that covered.
It would also be great if you could rebase, so we are sure there is nothing wrong with the new unit handling.
|
|
||
| if (minimum == null && (newValue = fragment.getMaximum()) != null) { | ||
| minimum = new QuantityType(newValue, newUnit).toInvertibleUnit(oldUnit).toBigDecimal(); |
There was a problem hiding this comment.
While this works, I would prefer a more explicit style here like:
| if (minimum == null && (newValue = fragment.getMaximum()) != null) { | |
| minimum = new QuantityType(newValue, newUnit).toInvertibleUnit(oldUnit).toBigDecimal(); | |
| BigDecimal newMinimum = fragment.getMaximum(); | |
| if (minimum == null && newMinimum != null) { | |
| minimum = new QuantityType(newMinimum, newUnit).toInvertibleUnit(oldUnit).toBigDecimal(); |
The same of course also applies to the other locations.
|
If I understand this correctly, bounds (and step) use the unit of the state pattern if set. However, this would probably have no effect, if the pattern is set to |
|
I've not had time to look at this again, and likely won't in the near future. I do agree with @boehan that it would make more sense if the StateDescription carried the unit with it as a QuantityType, rather than assuming the consumer knows how to parse the unit out of the format pattern (if provided). But I suspect that would be a significant breaking change. |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/uom-for-state-min-and-max-values-in-channel-description-xml/159863/2 |
See the comments in StateDescriptionFragmentImpl.merge for a full description of the problem and solution.